Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADIOS2 Feedstock #8526

Merged
merged 8 commits into from
Jul 1, 2019
Merged

ADIOS2 Feedstock #8526

merged 8 commits into from
Jul 1, 2019

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jun 8, 2019

Prepare a feedstock for ADIOS2, including MPI variants.

We will enable aarch64 and ppc64le support later on in the feedstock.

Checklist

  • License file is packaged (see here for an example)
  • Source is from official source
  • Package does not vendor other packages (it does for the start, but only private dependencies. we will reduce those over time)
  • Build number is 0
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/adios2) and found it was in an excellent condition.

@ax3l
Copy link
Member Author

ax3l commented Jun 9, 2019

@chuckatkins in coordination with @williamfgc, I have adopted your conda scripts for conda-forge.

There are still some smaller hiccups, but at least on Linux and OSX the build passes already.
I am currently using ADIOS2 v2.4.0rc1, so that we can carve out issues towards the 2.4.0 release.

Both of you, please confirm that you want to be listed as co-maintainers of the feedstock and please tell me anyone you want to add as well.

Prepare a feedstock for ADIOS2, including MPI variants.
@chuckatkins
Copy link

I agree to be listed as co-maintainer of this feedstock

@ax3l
Copy link
Member Author

ax3l commented Jul 1, 2019

@chuckatkins thanks!

@williamfgc it will be conda install -c conda-forge adios2 for the start. We also build MPI-variants which allows users to make the following installation choices:

  • conda install adios2 installs by default the nompi variant
  • conda install adios2=*=mpi* installs any mpi-compatible implementation
  • conda install adios2=*=mpi_mpich* explicitly picks mpich variant.
    This will have the same result as conda install mpich adios2=*=mpi*
  • conda install adios2=2.4.0=nompi* explicitly forbids mpi. Usually not necessary, as the build number offset will cause nompi to be preferred unless mpi is explicitly requested.

@ax3l
Copy link
Member Author

ax3l commented Jul 1, 2019

I'll move Windows HDF5 support to an in-feedstock update since I cannot spot the linker error here: https://ci.appveyor.com/project/conda-forge/staged-recipes/builds/25643672

@ax3l ax3l force-pushed the topic-adios2 branch 8 times, most recently from 1a1eef6 to a7f0d49 Compare July 1, 2019 09:41
Copy link
Member

@chrisburr chrisburr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few cosmetic things then LGTM 👍

recipes/adios2/meta.yaml Outdated Show resolved Hide resolved
recipes/adios2/meta.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,201 @@
Apache License
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this file? I think it should be in the source tarball

Copy link
Member Author

@ax3l ax3l Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we should always add the LICENSE file separately?
(link in PR description template)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always need to package it but we prefer to get it from source tarball(s) instead of including it in the recipe directory if we can to ensure it's correct and up to date.

Clean up: suggestions by Chris (thanks!)

Co-Authored-By: Chris Burr <[email protected]>
@ax3l
Copy link
Member Author

ax3l commented Jul 1, 2019

Thanks for the review @chrisburr, I added your suggestions and CI passed again :)

@ax3l
Copy link
Member Author

ax3l commented Jul 1, 2019

ping @chrisburr :)

CMAKE_PLATFORM_FLAGS+=(-DADIOS2_USE_BZip2=OFF)
elif [[ ${target_platform} =~ linux.* ]]; then
# link transitive libraries during build of intermediate wrapper lib
# export LDFLAGS="${LDFLAGS} -Wl,-rpath-link,${PREFIX}/lib"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these comments still needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Luckily not, will remove.

# prioritize nompi variant via build number
{% if mpi == 'nompi' %}
{% set build = build + 100 %}
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like the right way to do this.

Copy link
Member Author

@ax3l ax3l Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not? That's how @minrk showed it to me (hdf5, h5py et al.). We increase the build number for nompi variants to select them by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? That is kinda nuts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have documentation for this do we?

Copy link
Member Author

@ax3l ax3l Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this way of doing things is causing issues in the solver with multi output recipes that depend on packages with MPI variants. I've not managed to make sense of it yet but these builds show it:

Copy link
Member

@minrk minrk Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have documentation for this do we?

It's in the docs

Build offset is the only way to indicate preference in the solver that I'm aware of that works for existing packages. Note that the original and first-implemented plan was to use track_features to deprioritize, but this does not work for packages that have ever been published before, but I think it might work for new packages, or packages where all previous builds have been fully revoked or hotfixed.

In most cases, packages that require MPI express no preference, but it's more complicated when you have three variants and want users to opt-in to MPI (e.g. hdf5). Conda still has no ability to express preferences in a natural way, so any solution is going to be a bit wonky until there's a real solution. For that reason, I might recommend avoiding a 'preference' unless it's important to avoid picking up a variant in certain circumstances, at least until there's a better way.

@ax3l ax3l force-pushed the topic-adios2 branch 3 times, most recently from 5e48a18 to d654b04 Compare July 1, 2019 17:29
@chrisburr chrisburr merged commit 41589a2 into conda-forge:master Jul 1, 2019
@ax3l ax3l deleted the topic-adios2 branch July 1, 2019 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants